Skip to content

Error view #215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 22 commits into from
Closed

Error view #215

wants to merge 22 commits into from

Conversation

theJasonHelmick
Copy link
Contributor

No description provided.

```powershell
<CommandName>:<Exception Message>
get-item: Cannot find path ‘C:\blah’ because it does not exist:

Copy link
Contributor

@KirkMunro KirkMunro Aug 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop the position and make this the concise/simple view. In my experience, when users see line numbers, that, coupled with red text, encourages them to go straight to the script developer.

Or better, make it conditional. Only show the position if it comes from a file. Otherwise, it is not needed in the concise/simple view.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, don't include position (in the default output) unless it's related to an actual file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3rded and like the conditional

@Jaykul
Copy link
Contributor

Jaykul commented Aug 2, 2019

I still think we should make this more extensible...

Instead of just adding one new value (and further complicating the format code) ... move the code that's in the format file into commands, with one command for each "ErrorView" like ConvertTo-CategoryView -- then, in the format code, just look for a ConvertTo-$ErrorView command and use it.

Alternatively, make the views named "Custom" views, and have the default format code call Format-Custom ...

Either way, people can add new views trivially by just writing a format function and setting the variable to the matching string. You can see an example of that in the ErrorView module.

@joeyaiello joeyaiello added this to the 7.0-Consider milestone Aug 12, 2019
$error[1] | Resolve-ErrorRecord

# Display detailed error information for the last 5 errors
Resolve-ErrorRecord -Last 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you originally had -Last but changed to -Newest without updating the examples

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The examples still show -First and -Last, but the cmdlet definition on line 71 only shows -Newest. I think keeping just -Newest makes sense so the examples need to be updated.

@OneCyrus
Copy link

how about a more visualized error message? i really like the approach some web development tools try to give you more context about the error and point you in the correct direction.

here's an example i did which is inspired by react overlay error

image

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial comments have been addressed

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error formatting even for simple view can take some lessons from https://blog.rust-lang.org/2016/08/10/Shape-of-errors-to-come.html which can make the information more easily consumable for users. Simple doesn't have to mean less lines, but less but relevant information.

$error[1] | Resolve-ErrorRecord

# Display detailed error information for the last 5 errors
Resolve-ErrorRecord -Last 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The examples still show -First and -Last, but the cmdlet definition on line 71 only shows -Newest. I think keeping just -Newest makes sense so the examples need to be updated.

@KirkMunro
Copy link
Contributor

There are quite a few discussions that were started in this RFC that are marked as resolved without a resolution (comment, update to the RFC, etc.). That combined with some of the other comments made in here makes it feel like you're talking around the community rather than responding to it in certain places. Please share internal discussions/resolutions/comments in the comment system in this RFC so that if something is resolved due to an internal discussion, we have visibility into that resolution and can respond back if necessary.

| ^^^ Cannot find path 'C:\notreal' because it does not exist.
|
* Help: this is for additional help information

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention of this RFC is solid, but that output seems very confusing and does not feel like a step forward at all.

Copy link
Contributor

@vexx32 vexx32 Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree... it has a lot of wasted space, and it's not immediately clear that the number is a line number. Thinking out loud as it were:

PS C:\> .\MyScript.ps1
ERROR: Cannot find path 'C:\notreal' because it does not exist.
  ---> C:\GitHub\pri-errorview\RustTest\test.ps1
L15> | Get-ChildItem -Path c:\notreal 
                           ~~~~~~~~~~:> Cannot find path 'C:\notreal' because it does not exist.
    * Help: this is for additional help information

It's difficult to fit the necessary information in as little space as possible. Consider also that some error messages are very long, so displaying them at the end of a line rather than on a new line can make them harder to read as well.

* Help: this is for additional help information

PS C:\> $error[0] | Resolve-ErrorRecord

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are comments about Resolve-ErrorRecord in this RFC that are being ignored. Additional comments have been added calling that out, and those comments plus the others they refer to are still being ignored. Why is that happening???

With respect to Resolve-ErrorRecord, how is what that would return different from a custom error format? It's just outputting a string, not an object, right? How about having an error view specific for that detailed error format? What about the suggestion for a Get-Error command instead that could be used for more than just error resolution, but also error processing, with a Format-Error command to allow users to see whatever format they want? This has been suggested elsewhere, but since that part of the discussion is being ignored, I feel I need to repeat it again here.

@theJasonHelmick
Copy link
Contributor Author

Based on many of the ideas in this conversation, a simple update to this RFC would not allow for further comment. To better address those concerns and open the RFC for comment, I am closing this pr and having created a new pr #228 that will be open for comment until 10/31/19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.